Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix The conhost command line is not properly escaped #1815

Merged
merged 4 commits into from
Jul 12, 2019
Merged

Fix The conhost command line is not properly escaped #1815

merged 4 commits into from
Jul 12, 2019

Conversation

fcharlie
Copy link
Contributor

@fcharlie fcharlie commented Jul 4, 2019 •
edited
Loading

Summary of the Pull Request

This PR is used to fix conhost when the process is started, and the command line is not properly escaped causing an error. For details, please see Issues: #1090

References

PR Checklist

  • Closes Bug Report: The conhost command line is not properly escaped #1090
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@fcharlie
Copy link
Contributor Author

fcharlie commented Jul 4, 2019

ConsoleArgumentsTests::ArgSplittingTests is bad..

@fcharlie fcharlie mentioned this pull request Jul 4, 2019
Closed
miniksa
miniksa requested changes Jul 5, 2019
View reviewed changes
src/host/ConsoleArguments.cpp Outdated Show resolved Hide resolved
src/host/ConsoleArguments.cpp Outdated Show resolved Hide resolved
src/host/ut_host/ConsoleArgumentsTests.cpp Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 5, 2019
@DHowett-MSFT
Copy link
Contributor

The core issue is that we are relying on the command line arguments coming in pre-mangled by the CRT’s argument parser.

It should be possible to, when we detect that we should switch to “client command line” parsing, just call GetCommandLineW and take the raw unmangled argument string back through CreateProcess.

Would that be a cleaner design that doesn’t require us to re-escape un-escaped strings?

@DHowett-MSFT
Copy link
Contributor

Oh, it looks like we aren’t relying on the CRT. Shouldn’t this be easier, then? We should just not mangle the arguments only to have to unmangle them later.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 5, 2019
@fcharlie
Copy link
Contributor Author

fcharlie commented Jul 5, 2019 •
edited
Loading

@DHowett-MSFT Using the original command line arguments is possible in this project (but the Windows SDK UCRT still needs to be escaped to fix spawnv ), but using the original command line to calculate the offset position of the parameters that conhost can handle will be cumbersome.

In addition, the 'escape' code learned the golang source. and is a mature solution.

https://github.com/golang/go/blob/master/src/syscall/exec_windows.go#L26-L95

@fcharlie
Copy link
Contributor Author

@miniksa The PR has been updated, but the CI is not responding.

@DHowett-MSFT
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@fcharlie
Copy link
Contributor Author

@DHowett-MSFT Thanks.

@fcharlie fcharlie mentioned this pull request Jul 10, 2019
Closed
DHowett-MSFT
DHowett-MSFT approved these changes Jul 12, 2019
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

@DHowett-MSFT DHowett-MSFT merged commit 02e8389 into microsoft:master Jul 12, 2019
@fcharlie
Copy link
Contributor Author

@DHowett-MSFT Thanks. Windows Terminal makes me happy

@fcharlie fcharlie mentioned this pull request Jul 12, 2019
Closed
@fcharlie fcharlie deleted the conhost_commandline_escape branch July 12, 2019 04:01
mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 23, 2019
@fcharlie @mcpiroman
Fix the conhost command line not being properly escaped (microsoft#1815)
fcf4576 
This commit re-escapes the path to conhost's subprocess before it launches it.
@ghost
Copy link

ghost commented Aug 3, 2019

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@miniksa miniksa miniksa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug Report: The conhost command line is not properly escaped
3 participants
@fcharlie @DHowett-MSFT @miniksa